feat: DH-9378: Web Keyed Selection - #2736
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Key collisions, incorrect off-viewport resolution, model-swap handling, and keyboard deselection remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/iris-grid/src/KeyedSelection.ts:30
- These sentinels still make distinct valid keys collide: a numeric
NaNserializes identically to the string"__NaN__"(and likewise for both infinities). Selecting either group will therefore select/filter/copy both. Use an injective, type-tagged encoding rather than values that can also occur as user strings.
packages/iris-grid/src/IrisGridTableModelTemplate.ts:1666 - These subscription bounds are grid-row indices, but when totals are shown on top each data row is shifted by
floatingTopRowCount(row()compensates for this at lines 999-1014). Resolving an out-of-viewport keyed range therefore fetches the following raw table rows and commits the wrong keys. Translate requested grid ranges to underlying table rows (and handle totals/pending rows separately) before subscribing and filtering the result.
- Files reviewed: 45/54 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Key collisions and inverted off-viewport handling can select incorrect rows, while sparse range resolution can fetch enormous envelopes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
packages/iris-grid/src/IrisGridTreeTableModel.ts:122
- “Ensures” is the intended word here; “insures” refers to providing insurance.
packages/iris-grid/src/IrisGridTableModelTemplate.ts:1666
- Fetching one bounding envelope makes sparse programmatic selections scale with the distance between ranges rather than the number of selected rows. Selecting rows 1 and 1,000,000,000 requests and materializes the entire billion-row viewport just to retain two keys. Consolidate requested ranges and fetch them individually or in bounded chunks instead of subscribing to the full envelope.
packages/iris-grid/src/KeyedSelection.ts:29 - This encoding is not injective: a numeric
NaNand the string"__NaN__"both serialize as["__NaN__"](likewise for the infinity sentinels), andundefinedalso collides withnullinside the array. Rows with these distinct key values will therefore select, deselect, copy, and export as one key group. Use a type-tagged, escaped canonical encoding and add collision cases for every supported key type.
- Files reviewed: 45/54 changed files
- Comments generated: 2
- Review effort level: Balanced
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
Liking the look of this a lot better.
| * True when the selection covers exactly one row. | ||
| */ | ||
| toActiveRanges: () => readonly GridRange[]; | ||
| /** Column `[start, end]` pairs for scrollbar tick rendering. */ | ||
| getColumnTickRanges: () => readonly BoundedAxisRange[]; | ||
| /** Row `[start, end]` pairs for scrollbar tick rendering. */ | ||
| getRowTickRanges: () => readonly BoundedAxisRange[]; | ||
| isSingleRowSelection: () => boolean; | ||
| /** | ||
| * The single selected visible row, or `null` when zero or multiple rows | ||
| * are selected. Drives `gotoRow` sync. | ||
| */ | ||
| getLastSingleSelectedRow: () => VisibleIndex | null; |
There was a problem hiding this comment.
These two still seem a little odd for the abstraction. getLastSingleSelectedRow is only used by IrisGrid (not used by Grid proper itself):
const singleRow = selection.getLastSingleSelectedRow();
if (singleRow != null) {
this.setState({ gotoRow: `${singleRow + 1}` });
}Elsewhere in IrisGrid, when setting gotoRow, we do it based on the cursorRow of the grid itself:
const cursorRow = this.grid?.state.cursorRow;
const cursorColumn = this.grid?.state.cursorColumn;
...
this.setState({
gotoRow: `${cursorRow + 1}`,
...
});
}Would it make sense to instead of the Selection have a cursorCell property that returns the cursor column/row - I think that could be used here instead of getLastSingleSelectedRow() (and also then could be used instead of accessing cursorRow/cursorColumn directly from Grid, keeping the source of truth in the Selection object itself). (Should those even be removed from the Grid state and be part of Selection? Though maybe a bigger change than we want now).
Then isSingleRowSelection is used by the SelectionKeyHandler for the same thing twice:
// Avoid deselection when the target cell is already selected in a single-row selection.
if (
!isShiftKey &&
grid.state.selection.isSingleRowSelection() &&
grid.state.selection.isCellSelected(targetColumn, targetRow)
) {
grid.handleKeyMoveCursor(targetColumn, targetRow);
return true;
}
I actually don't think this logic is correct, or something is wrong here anyways. If I'm holding Ctrl, I should be able to click, then click and drag another selection, and keep adding to it, e.g. like our old behaviour:
Screencast.from.2026-09-03.10-13-02.mp4
However, it seems if I click and drag now, it's reverting the selection:
Screencast.from.2026-09-03.10-14-01.mp4
So I think first need to clean up that behaviour, we may not even need this last single row selection at all. We should also add a test for this case.
| @@ -216,7 +222,7 @@ export type GridState = { | |||
There was a problem hiding this comment.
So thinking about my other comment about cursorRow/cursorColumn - should these be deprecated as well, and instead codified into the Selection? Seems like it would make more sense to have the Selection be the source of truth about the selection.
Perhaps could argue that cursor is not necessarily selection; but I think it is?
There was a problem hiding this comment.
Should have some unit tests around this file/behaviour
There was a problem hiding this comment.
🟡 Changes recommended
Unsettled mouse gestures and stale or unsupported keyed-selection states can produce incorrect copy, export, and selection behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/iris-grid/src/IrisGrid.tsx:3710
- Comparing only keyed versus non-keyed preserves a
KeyedSelectionwhen a schema changes from one non-empty key definition to another. Its serialized keys and rawselectedKeyValuesstill belong to the old key columns, so rendering may highlight wrong rows and copy/download may filter the new columns with old values. Clear keyed selections on schema changes, or retain and compare the exact key-column identity before preserving them.
const modelIsKeyed = isKeyedGridModel(model);
const selectionIsKeyed = currentSelection instanceof KeyedSelection;
if (modelIsKeyed !== selectionIsKeyed) {
this.grid?.clearSelectedRanges();
packages/iris-grid/src/IrisGridTableModelTemplate.ts:1666
- This bounding-envelope subscription fetches every row between disjoint requested ranges. A programmatic selection of two distant rows can therefore materialize millions or billions of intervening rows even though they are discarded below, causing severe latency or memory pressure. Fetch consolidated ranges separately or use a sparse row-set/snapshot API instead of one contiguous envelope.
- Files reviewed: 46/55 changed files
- Comments generated: 3
- Review effort level: Balanced
| grid.moveCursorToPosition(column, row); | ||
| grid.commitSelection(); | ||
| // Double-click behaves as a plain click at the target: replace selection. | ||
| grid.handleMouseSelectStart({ row, column }, 'replace'); |
| this.stopTimer(); | ||
| grid.clearSelectedRanges(); | ||
| grid.moveCursorToPosition(gridPoint.column, gridPoint.row); | ||
| grid.handleMouseSelectStart({ row, column }, 'replace'); |
| get selectionKeyColumnIndices(): readonly ModelIndex[] { | ||
| return this.getMemoizedSelectionKeyColumnIndices( | ||
| this.columns, | ||
| (this.table as DhType.Table).getAttribute?.('keyColumns') | ||
| ); |
Implements keyed row selection for iris-grid.
See ticket for test code in the test plan.